Blocks: Propose a server side rendering implementation #586

youknowriad · 2017-05-01T14:01:46Z

This is also called dynamic blocks and is useful for blocks like embed, comments blocks, ...
This is still in progress but feedback is appreciated.

Testing instructions

Insert this anything to a post content (old editor) and preview this post, you should see your block replaced by THIS IS AWESOME

aduth · 2017-05-01T14:20:39Z

index.php

+ */
+function register_block( $name, $render ) {
+	global $registered_blocks;
+	$registered_blocks[ $name ] = $render;


I don't want to preemptively assume requirements, but do you think we'd need a block to register options aside from just a render?

I tried to think about this, but I didn't find any use-case for now. Though, we don't know the future maybe options is preferable.

aduth · 2017-05-01T14:21:14Z

index.php

+
+ * @return string          Updated post content
+ */
+function do_dynamic_blocks( $content ) {


This function could do for some comments; various indices are hard to follow.

Is the specificity of "dynamic" something we want to expose in the function name? I'd expect we'll add hooks here for interacting with individual blocks (say I want to filter all quote blocks for some reason). Performance here will be very important as well.

aduth · 2017-05-01T14:21:45Z

index.php

+	$new_content = $content;
+	foreach ( $matches[ 0 ] as $index => $block_match ) {
+		$block_name = $matches[ 1 ][ $index ][ 0 ];
+		if ( isset( $registered_blocks[ $block_name ] ) ) {


Maybe worth an early continue to avoid indentation:

if ( ! isset( $registered_blocks[ $block_name ] ) ) { continue; }

aduth · 2017-05-01T14:22:25Z

index.php

+	foreach ( $matches[ 0 ] as $index => $block_match ) {
+		$block_name = $matches[ 1 ][ $index ][ 0 ];
+		if ( isset( $registered_blocks[ $block_name ] ) ) {
+			$block_length = mb_strlen( $block_match[ 0 ] );


I don't know the answer, but is mb_strlen the safe default over strlen for multi-byte content?

In my PHP souvenirs, it is, but I'm not in my comfort zone anymore in PHP. Other 👀 here could be useful.

aduth · 2017-05-01T14:25:23Z

index.php

@@ -247,3 +316,7 @@ function the_gutenberg_project() {
 	</div>
 	<?php
 }
+
+register_block( 'core/html', function( $attributes ) {


Seems temporary, but just for clarity, the anonymous function callable is only available in PHP versions 5.3.0 and newer (reference). WordPress supports as far back as 5.2.4.

Oh right 👍

aduth · 2017-05-01T14:25:53Z

index.php

+ */
+function do_dynamic_blocks( $content ) {
+	global $registered_blocks;
+	$open_matcher = '/<!--\s*wp:([a-z](?:[a-z0-9\/]+)*)\s+((?:(?!-->).)*)-->.*<!--\s*\/wp:\g1\s+-->/';


Would we want the registered block to be able to make use of the content between the comment delimiters?

I know there were some discussions about this, but IMO for consistency, Dynamic blocks should not contain any data between the delimiters. Their save function should return null.

We shouldn't prescribe what save returns, because it may make sense to return a link, or a short message. Consider a block that renders the last 5 recent posts from a certain category. It may be good that when the dynamic markup can't be generated, it returns a link to /blog/category/:term with some text saying "View latest posts on :term".

mtias · 2017-05-01T17:44:16Z

Thanks for starting this.

youknowriad · 2017-05-02T09:13:09Z

I'm thinking to leverage this for the embed block. But to do so, we need to change the way we store attributes (use HTML like syntax).

What do you think if

we remove the dummy "register_block" call.
Try to merge this proposal
do a follow-up PR to change the attributes' syntax
and then another PR to add the server side embed block.

(I like splitting/Iterating :) )

aduth · 2017-05-02T19:12:21Z

I'm not totally clear what you mean by changing attribute syntax, but I'd be fine with merging this and iterating on it in subsequent pull requests since it has no impact on existing behavior for the moment.

nylen · 2017-05-02T23:13:57Z

This logic looks fairly complicated and needs unit tests as soon as possible. See https://pippinsplugins.com/unit-tests-wordpress-plugins-setting-up-testing-suite/ for example.

I think we should do more to unify our block registration APIs between the client and server (require the same rules for block slugs, for example). I also wonder if we need or want some way to guarantee that if a block is registered on the server, then it's also registered on the client (and the relevant details match up).

I'm not sure those need to be done in this PR, but I'm also not sure why we need to merge this immediately. Are we ready to use it yet?

BE-Webdesign · 2017-05-03T00:48:54Z

What does the server side API need to do, why does it need to do it, and how will it interact with the client side API, would be good questions to answer before we get too attached to this. I am fine with merging, but I think the current client API could use improvements right now as well, and we should circle back to refining the API at some point.

westonruter · 2017-05-03T02:41:04Z

Additionally, I think there is also the question as to whether the server-side API should also be able to facilitate rendering of blocks for displaying inside of the visual editor, similar to what the Shortcake plugin does. A good case for server-side rendering for the block in the editor is both for legacy content (e.g. shortcodes) but also for displaying dynamic blocks that are not feasible to provide pure JS renderings without duplicating a lot of logic.

youknowriad · 2017-05-03T09:19:43Z

@aduth

I'm not totally clear what you mean by changing attribute syntax

moving from attr:value to attr="value" or { attr: "value" }. We can't store URLs for example for now.

@nylen

This logic looks fairly complicated

I don't think so

needs unit tests as soon as possible

I agree

I think we should do more to unify our block registration APIs between the client and server

Sounds good for the validation

. I also wonder if we need or want some way to guarantee that if a block is registered on the server, then it's also registered on the client

I don't think so, we only need the dynamic blocks on the server, why forcing plugin authors to add something optional

but I'm also not sure why we need to merge this immediately

I'm not so impatient to merge this, I was waiting for review and you raise some good points here. But I like to split the work to smaller PRs.

Are we ready to use it yet?

Yes we are, we can use this to generate the HTML for the embed block for example.

@BE-Webdesign

What does the server side API need to do, why does it need to do it

The only thing that it needs to do for now is rendering dynamic blocks and we currently have a dynamic block on the client side (The embed block). I'm trying to start the server side implementation to tackle this particular task. We'll iterate and see if we need any other server side API.

@westonruter

Additionally, I think there is also the question as to whether the server-side API should also be able to facilitate rendering of blocks for displaying inside of the visual editor

I'm not sure I understand! Are you saying we should generate a form in the visual editor server-side? I would personally strongly argue against that and instead, build a specific "nice" UI for each block. But I understand this could be helpful for mobile apps maybe. Anyway, this is a separate task and for now, I'm trying to address the server-side rendering of dynamic blocks.

nylen · 2017-05-03T19:29:12Z

I'm not so impatient to merge this, I was waiting for review and you raise some good points here. But I like to split the work to smaller PRs.

I agree, I guess it makes me a bit nervous that we are building a copy of an API that already exists on the client side. We need to be careful that it works the same way as much as possible. To me that means we should follow a similar process here to #289, including the same test cases. So I'd prefer that we finish and merge #617 (PHPUnit) first, then add test cases here.

Edit: I see that you already did a lot of these updates, nice 😀

youknowriad · 2017-05-05T08:57:55Z

Server side tests in place, any other concern here?

nylen · 2017-05-05T12:26:05Z

This was also present on #617, but I'm seeing a lot of warnings in the build log:

PHP Warning: filemtime(): stat failed for /home/travis/build/WordPress/gutenberg/i18n/build/index.js in /home/travis/build/WordPress/gutenberg/index.php on line 161

Example: https://travis-ci.org/WordPress/gutenberg/jobs/228752501

Also, from #617 (comment), was this ever addressed?

Take a close look at each of the jobs, there are two showing "green" that they passed, this is false, there's lots of errors on those jobs.

Let's fix the warnings here and make sure that when the build fails it actually fails like it should. I'll do a separate PR against this branch to test out the second thing.

youknowriad · 2017-05-05T12:29:26Z

Oh! I haven't seen those errors. I thought he was talking about the failing jobs. At that moment three of five jobs were failing.

nylen · 2017-05-05T12:31:52Z

phpunit/class-dynamic-blocks-render-test.php

+	/**
+	 * Successfull dynamic block rendering
+	 */
+	function test_register_block() {


In this PR there have been issues with matching the content inside the block delimiters. Let's get another test similar to this one but testing out content inside blocks as well:

before this should be ignored between this should be ignored also after

This is also called dynamic blocks and is usefull for blocks like embed, comments blocks, ...

- Also, ensure PHP 5.2 compatibility - Adding some comments to explain the server side rendering

* Setup server side unit tests

nylen · 2017-05-05T13:37:18Z

index.php

+		return;
+	}
+
+	if ( isset( $registered_blocks[ $slug ] ) ) {


Why not trigger a _doing_it_wrong here too, like we do on the client side?

nylen · 2017-05-05T13:37:29Z

index.php

+ */
+function unregister_block( $slug ) {
+	global $registered_blocks;
+	if ( ! isset( $registered_blocks[ $slug ] ) ) {


Why not trigger a _doing_it_wrong here if this is set, like we do on the client side?

nylen · 2017-05-05T13:41:32Z

Here are the block registration test cases on the client side:

  blocks
    registerBlock()
      ✓ should reject numbers
      ✓ should reject blocks without a namespace
      ✓ should reject blocks with invalid characters
      ✓ should accept valid block names
      ✓ should prohibit registering the same block twice
      ✓ should store a copy of block settings
    unregisterBlock()
      ✓ should fail if a block is not registered
      ✓ should unregister existing blocks

Let's get all of the same tests running here too.

- Do the client-side build before running PHPUnit tests; this ensures that the plugin doesn't try to enqueue non-existent JS files and cause a bunch of warnings - If a step in the build fails, go ahead and exit - Don't scan node_modules/ and vendor/ for PHP files - Run phpcs with the `-s` option (show rule identifiers in reports) - Don't require docblocks for unit tests

youknowriad · 2017-05-08T09:14:24Z

I've added more unit tests fc7fabe (closer to what we have on the frontend). Can I have another look @nylen?

Before this change, running the unit tests in this configuration required an extra step (`WP_TESTS_DIR=... phpunit`).

nylen · 2017-05-09T17:27:50Z

There were still a couple of tests missing:

should prohibit registering the same block twice
Blocks: Propose a server side rendering implementation #586 (comment)
verify that the functions actually make the expected changes to the blocks global variable

Other changes:

As of 47e81bd every test method no longer needs to have an annotation.
Changed $registered_blocks to $wp_registered_blocks; in WP, prefix everything.
Explicit return false; for error conditions; also test accordingly.
You can now run phpunit from within a WP dev install without additional configuration or having to specify WP_TESTS_DIR.

nylen

If latest changes look OK:

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 1, 2017

youknowriad self-assigned this May 1, 2017

youknowriad requested review from mtias and aduth May 1, 2017 14:01

mtias added the [Feature] Block API API that allows to express the block paradigm. label May 1, 2017

aduth reviewed May 1, 2017

View reviewed changes

youknowriad mentioned this pull request May 4, 2017

Setup server side unit tests #617

Merged

nylen reviewed May 5, 2017

View reviewed changes

youknowriad added 5 commits May 5, 2017 08:36

Blocks: Propose a server side rendering implementation

6f26133

This is also called dynamic blocks and is usefull for blocks like embed, comments blocks, ...

Blocks: Registering a block server side takes an array

6e816dd

- Also, ensure PHP 5.2 compatibility - Adding some comments to explain the server side rendering

Blocks: Validate the registred block server side

f7ac9f3

SSR: Simplify the block server side rendering algorithm

e706f6e

Setup server side unit tests (#617)

96d6b3b

* Setup server side unit tests

nylen force-pushed the try/dynamic-blocks branch from 86e4acd to 96d6b3b Compare May 5, 2017 12:36

nylen mentioned this pull request May 5, 2017

Trigger some PHPUnit failures #676

Closed

nylen reviewed May 5, 2017

View reviewed changes

nylen and others added 3 commits May 5, 2017 16:59

SSR: add missing doing it wrong

db2b558

Testing: Add more registration unit tests

fc7fabe

nylen added 7 commits May 9, 2017 11:59

Prefix global name with wp_

33f0961

Use tearDown functions rather than unregistering blocks in each test

eb176ba

Test that $wp_registered_blocks global is modified as expected

d2e0c30

Add a missing test for register_block

44c4f5a

Explicitly return false for errors and test this condition

a2d6fee

Add test with (ignored) content inside block delimiters

bea95aa

Make it possible to run phpunit from within an existing WP dev install

b396229

Before this change, running the unit tests in this configuration required an extra step (`WP_TESTS_DIR=... phpunit`).

nylen added 2 commits May 9, 2017 12:31

Fix a logic error

9e6b1ef

Fix style in PHP comments

138eb69

nylen force-pushed the try/dynamic-blocks branch from 60442f6 to 138eb69 Compare May 9, 2017 17:31

nylen approved these changes May 9, 2017

View reviewed changes

youknowriad merged commit 109aecf into master May 10, 2017

youknowriad deleted the try/dynamic-blocks branch May 10, 2017 08:31

This was referenced May 24, 2017

Plugin: PHP wrapper to add new blocks #886

Closed

Role of React going forward #893

Closed

naiba4 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities naiba4/gutenberg#10

Open

yassine4999 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities yassine4999/gutenberg#13

Open

Verdinjoshua26 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#71

Open

monzerfoda mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities monzerfoda/gutenberg#12

Open

MaxMood96 mentioned this pull request Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities MaxMood96/gutenberg#100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Propose a server side rendering implementation #586

Blocks: Propose a server side rendering implementation #586

youknowriad commented May 1, 2017

aduth May 1, 2017

youknowriad May 1, 2017

aduth May 1, 2017

mtias May 1, 2017

aduth May 1, 2017

aduth May 1, 2017

youknowriad May 1, 2017

aduth May 1, 2017

youknowriad May 1, 2017

aduth May 1, 2017

youknowriad May 1, 2017

mtias May 3, 2017

mtias commented May 1, 2017

youknowriad commented May 2, 2017

aduth commented May 2, 2017

nylen commented May 2, 2017

BE-Webdesign commented May 3, 2017 •

edited

Loading

westonruter commented May 3, 2017

youknowriad commented May 3, 2017

nylen commented May 3, 2017 •

edited

Loading

youknowriad commented May 5, 2017

nylen commented May 5, 2017

youknowriad commented May 5, 2017

nylen May 5, 2017

nylen May 5, 2017

nylen May 5, 2017

nylen commented May 5, 2017

youknowriad commented May 8, 2017

nylen commented May 9, 2017

nylen left a comment

Blocks: Propose a server side rendering implementation #586

Blocks: Propose a server side rendering implementation #586

Conversation

youknowriad commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented May 1, 2017

youknowriad commented May 2, 2017

aduth commented May 2, 2017

nylen commented May 2, 2017

BE-Webdesign commented May 3, 2017 • edited Loading

westonruter commented May 3, 2017

youknowriad commented May 3, 2017

nylen commented May 3, 2017 • edited Loading

youknowriad commented May 5, 2017

nylen commented May 5, 2017

youknowriad commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented May 5, 2017

youknowriad commented May 8, 2017

nylen commented May 9, 2017

nylen left a comment

Choose a reason for hiding this comment

BE-Webdesign commented May 3, 2017 •

edited

Loading

nylen commented May 3, 2017 •

edited

Loading